drivers: ethernet: ksz8081: streamline codebase #81816
Conversation
Simplifies error handling in `phy_mc_ksz8081_get_link()` by centralizing mutex unlocking with a `done` label. Signed-off-by: Pisit Sawangvonganan <[email protected]>
Simplifies GPIO reset logic in `phy_mc_ksz8081_reset()` by operating within a conditional block, allowing the removal of the `skip_reset_gpio` label. Signed-off-by: Pisit Sawangvonganan <[email protected]>
| @@ -283,27 +276,27 @@ static int phy_mc_ksz8081_reset(const struct device *dev) | |||
| } | |||
|
|
|||
| #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) | |||
| if (!config->reset_gpio.port) { | |||
| goto skip_reset_gpio; | |||
There was a problem hiding this comment.
the rest of the PR is good but I preferred the negative check here to avoid the extra indentation (ie keeping indentation strictly max of 2 indents), maybe would be better to make the gpio reset into a helper function, like:
#if DT_ANY_INST_HAS_PROP_STATUS_OKAY
static int phy_ksz8081_reset_gpio(config *)
{
int ret;
if (!config->reset_gpio.port) {
return 0;
}
/* rest of reset code */
return ret;
}
#else
#define phy_ksz8081_reset_gpio(...)
#endif
There was a problem hiding this comment.
Introducing helper functions might cause ambiguity between a missing GPIO port (returning 0) and a successful GPIO operation (also returning 0).
Adding extra return values to differentiate them would complicate the code.
Given the constraint of maintaining a max of 2 indents, I suggest reverting to the previous code structure but improving it by moving const struct mc_ksz8081_config *config = dev->config; inside the #if block.
WDYT? :)
There was a problem hiding this comment.
There's already silence if there's a missing gpios port, maybe what you want to do is add some kind of log message
This PR includes the following improvements:
phy_mc_ksz8081_get_link()phy_mc_ksz8081_reset()